Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[stable29] Send one full update from y-websocket #6696

Merged
merged 17 commits into from
Nov 27, 2024
Merged

[stable29] Send one full update from y-websocket #6696

merged 17 commits into from
Nov 27, 2024

Conversation

backportbot[bot]
Copy link

@backportbot backportbot bot commented Nov 26, 2024 •
edited by mejo-
Loading

Backport of #6650

@backportbot backportbot bot added bug Something isn't working 3. to review feature: sync labels Nov 26, 2024
@backportbot backportbot bot added this to the Nextcloud 29.0.10 milestone Nov 26, 2024
@mejo- mejo- marked this pull request as ready for review November 26, 2024 15:00
juliusknorr
juliusknorr requested changes Nov 26, 2024
View reviewed changes
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's wait with the merge for our testing round tomorrow just to double check.

@mejo- mejo- self-requested a review November 26, 2024 15:41
@mejo- mejo- force-pushed the backport/6650/stable29 branch 5 times, most recently from 9f1da43 to 7028dc6 Compare November 27, 2024 11:22
@max-nextcloud
Copy link
Collaborator

I suspect it (also) does not like the typescript file (in src/services/Outbox.ts). We probably should turn this into a js file and remove all type annotations.

@mejo-
Copy link
Member

mejo- commented Nov 27, 2024

I suspect it (also) does not like the typescript file (in src/services/Outbox.ts). We probably should turn this into a js file and remove all type annotations.

Yes, done already. But the cypress component tests also failed due to a version bump of @nextcloud/webpack-vue-config in a former PR. I pushed a commit now to fix this.

@max-nextcloud @mejo-
fix(sync): inline y-websocket to track received updates there
d5f1afc 
Signed-off-by: Max <[email protected]>
@max-nextcloud @mejo-
fix(lint): y-websocket
25f37c3 
Signed-off-by: Max <[email protected]>
@max-nextcloud @mejo-
enh(y-websocket): always send full diff to server state
f7e9e5b 
Keep an internal ydoc tracking updates that came from the server.
Send updates, that would sync this doc with the current doc state.

Signed-off-by: Max <[email protected]>
@max-nextcloud @mejo-
fix(license): add license header and props to y-websocket
b2245ea 
Signed-off-by: Max <[email protected]>
@max-nextcloud @mejo-
fix(awareness): only send updates about the local client
2c52d80 
Signed-off-by: Max <[email protected]>
@max-nextcloud @mejo-
fix(sync): make use of steps in push responses
87038f0 
The pushed steps are echoed back with all other steps since version immediately.
Processing them reduces the size of the following pushes and syncs.

Signed-off-by: Max <[email protected]>
@max-nextcloud @mejo-
refactor(sync): rename _receiveSteps to receiveSteps
0b83dbb 
It has always been publicly called from the PollingBackend.

Signed-off-by: Max <[email protected]>
@max-nextcloud @mejo-
refactor(sync): drop superfluous update message
acfd535 
Updates now include all the local structs
that were not yet received from remote.

No need to compute a separate update message anymore.

Signed-off-by: Max <[email protected]>
@max-nextcloud @mejo-
fix(sync): Do not resend document state in first push.
20fbcac 
Apply document state as a step.
Process it like other steps received from the remote.
In particular include it in the tracking of steps already applied
and set the version accordingly.

Signed-off-by: Max <[email protected]>
@max-nextcloud @mejo-
fix(sync): reply to queries with steps since last save
27f4a2d 
This was a very inefficient attempt to resync
that we did not even process on the client side.

Only the steps since the last save may not be enough
to get back in sync.
However we can expand this by including the document state
or storing it as the first step after a save.

Signed-off-by: Max <[email protected]>
max-nextcloud and others added 7 commits November 27, 2024 14:33
@max-nextcloud @mejo-
fix(sync): include document state in response to queries
93235e2 
Signed-off-by: Max <[email protected]>
@max-nextcloud @mejo-
fix(sync): process document state from push response
568031d 
Do not process document state from create response.
During create the editor has not been initialized fully
and the cursor position is 0 - which is invalid
as it is not inside a node with inline content.
(It is inside the doc before the initial paragraph.)

This also allows processing document state later on
in order to recover from out of sync situations.
But we do not make use of that yet.

Signed-off-by: Max <[email protected]>
@max-nextcloud @mejo-
refactor(sync): cache steps in sync service
6c51c92 
Store the steps that need to be send
where we also do the debouncing.

They will be updated whenever there is a new message from y-websocket.

Signed-off-by: Max <[email protected]>
@max-nextcloud @mejo-
refactor(Editor): instantiate api outside sync service
a39864d 
Signed-off-by: Max <[email protected]>
@mejo-
test(cypress): Pass SessionApi into the sync service
ca62b85 
Signed-off-by: Jonas <[email protected]>
@max-nextcloud @mejo-
fix(SyncService): only log errors for now
cc6fcbd 
Signed-off-by: Max <[email protected]>
@mejo-
chore: Fix linter errors and warnings
9874d6f 
Signed-off-by: Jonas <[email protected]>
@mejo-
Copy link
Member

mejo- commented Nov 27, 2024

Failing conflicts.spec.js is unrelated. They're also flaky on stable30 at least and already failed in other stable29 PRs. It seems like they fail more reliable now though.

@mejo- mejo- merged commit 03d88e0 into stable29 Nov 27, 2024
61 of 63 checks passed
@mejo- mejo- deleted the backport/6650/stable29 branch November 27, 2024 15:18
@skjnldsv skjnldsv mentioned this pull request Nov 27, 2024
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juliusknorr juliusknorr juliusknorr requested changes

@mejo- mejo- mejo- approved these changes

@max-nextcloud max-nextcloud Awaiting requested review from max-nextcloud

Assignees

@max-nextcloud max-nextcloud

Labels
3. to review bug Something isn't working feature: sync
Projects
None yet
Milestone
Nextcloud 29.0.10
Development

Successfully merging this pull request may close these issues.
3 participants
@max-nextcloud @mejo- @juliusknorr